Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

streams: make streams lazy by default #673

Closed
wants to merge 1 commit into from

Conversation

calvinmetcalf
Copy link
Contributor

Makes streams lazy by default so crypto does not need to monky patch them in
order to get good performance. This helps the state objects become part of the
private api in #445.

Other discussion at nodejs/readable-stream#109

Ran the bench marks on crypto and they seemed to be compatible (unlike my first attempt) somebody should definitely check me on that.

Makes streams lazy by default so crypto does not need to monky patch them in
order to get good performance. This helps the state objects become part of the
private api in nodejs#445.
@chrisdickinson
Copy link
Contributor

The goal seems to be that one can subclass a stream, but avoid the cost of instantiating a stream when instantiating the class (or defer that until it's absolutely needed.)

My initial reaction to this is -0. It seems a bit like the error tracking that {wa,i}s being done in Writable streams for the sole benefit of net.Socket. I haven't totally made up my mind on this, though. I might be swayed.

@calvinmetcalf
Copy link
Contributor Author

@chrisdickinson so in crypto streams if you just use the sync update digest/final methods you never actually need to initialize the stream.

The general use case from a consumer api would be creating 'streamable' objects that can you a fast path for synchronous usage and a queued path for dealing with async data.

@vkurchatkin
Copy link
Contributor

-1. crypto needs it because it (hash, etc.) has dual interface. the real problem is that hash shouldn't be a transform: transform is supposed to output a stream. Hash is a single buffer with fixed length. It makes sense for it be Writable, but not readable.

@calvinmetcalf
Copy link
Contributor Author

Ciphers though

On Fri, Jan 30, 2015, 5:08 PM Vladimir Kurchatkin notifications@github.com
wrote:

-1. crypto needs it because it has dual interface. the real problem is
that hash shouldn't be a transform: transform is supposed to output a
stream. Hash is a single buffer with fixed length. It makes sense for it be
Writable, but not readable.


Reply to this email directly or view it on GitHub
#673 (comment).

@vkurchatkin
Copy link
Contributor

@calvinmetcalf my bad, you are right. Dual interfaces are still bad. Deprecating one and blessing the other is the way I like.

@chrisdickinson
Copy link
Contributor

Ideally I'd like to expose the streaming versions on top of the existing versions – as a cipher.createTransformStream() or hash.createWriteStream(), or (at least) not make it easier to create interfaces that are blended as this one currently is.

@calvinmetcalf
Copy link
Contributor Author

So currently hash does emit it's value in the stream but digest and verify
don't so with this could be turned into write streams

On Fri, Jan 30, 2015, 5:22 PM Chris Dickinson notifications@github.com
wrote:

Ideally I'd like to expose the streaming versions on top of the existing
versions – as a cipher.createTransformStream() or hash.createWriteStream(),
or (at least) not make it easier to create interfaces that are blended as
this one currently is.


Reply to this email directly or view it on GitHub
#673 (comment).

@calvinmetcalf
Copy link
Contributor Author

Er sign and verify

On Fri, Jan 30, 2015, 6:08 PM Calvin Metcalf calvin.metcalf@gmail.com
wrote:

So currently hash does emit it's value in the stream but digest and verify
don't so with this could be turned into write streams

On Fri, Jan 30, 2015, 5:22 PM Chris Dickinson notifications@github.com
wrote:

Ideally I'd like to expose the streaming versions on top of the existing
versions – as a cipher.createTransformStream() or
hash.createWriteStream(), or (at least) not make it easier to create
interfaces that are blended as this one currently is.


Reply to this email directly or view it on GitHub
#673 (comment).

@Fishrock123 Fishrock123 added the discuss Issues opened for discussions and feedbacks. label Feb 5, 2015
@chrisdickinson
Copy link
Contributor

Closing this. Lazy initialization encourages more mixed-usage APIs, which we don't want to encourage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants